I still find it odd that we have to do the "unbound" check everywhere where we call .member. Do we not always want to emit a diagnostic when failing to resolve a member?

We do, but it will need to be different error codes. It would be very counterintuitive for users if import foo triggers an unresolved-import error code and from foo import bar triggers an unresolved-member error code. Users will expect all import-related failures to be the same error code and all attribute-access-related failures to be a different error code.

That makes sense. We might just want to parameterize the member method. But maybe that's not worth it? I don't know. Have you looked at how mypy/pyright do this?

We might just want to parameterize the member method.

Not sure exactly what you mean; could you clarify?

Have you looked at how mypy/pyright do this?

Mypy has a very different architecture to the one we're building. It resolves all imports eagerly in a first pass before it does any other type checking; module-not-found errors are emitted during this first pass here: https://github.com/python/mypy/blob/fe15ee69b9225f808f8ed735671b73c31ae1bed8/mypy/build.py#L2600-L2700.

I'm not as familiar with pyright's codebase but I can dig in now

Hmm... it seems both mypy and pyright in fact do what you'd prefer here -- they both use "attribute-access" error codes for from x import y imports:

• pyright: https://pyright-play.net/?pyrightVersion=1.1.368&code=GYJw9gtgBAxmA28CmMAuBLMA7AzgOgEMAjGKdCABzBFSmDDCA
• mypy: https://mypy-play.net/?mypy=latest&python=3.12&gist=3110174b575f8d8eba319462c45a8ddf

Pyright's error code is reportAttributeAccessIssue; mypy's is attr-defined, which both seem equally bad. In terms of error messages, pyright definitely wins, though: pyright has:

"foo" is unknown import symbol

whereas mypy has

Module "collections.abc" has no attribute "foo" 

I'm somewhat surprised by this. But given this precedent, I'm okay with emitting the diagnostic from .member(), if you'd prefer. Though I hope we still aspire to be more user-friendly in our error messages than either mypy or pyright.

Oh, I agree that that makes a lot of sense! I can make that change

Hmmm... if the inferred type of a member access is a union, e.g. int | Unbound, would you expect that to be represented as an Ok() or an Err() variant (if we return an Result from .member()?

having a closure or similar to create a diagnostic if the member is unbound.

This might be doable, but after looking at it for a little bit I think it would still be quite a large refactor and design change. I think it's out of the scope of this PR.

I'm not convinced there is any problem with reporting an error on from foo import bar (where the module foo does exist, but has no top-level symbol bar) as an attribute-access error code. If I had to pick between the pyright or mypy error messages above, I would prefer the mypy one (the pyright one gives less useful context). The fact that the attribute access occurred as part of a from-import will be clear from the error location; it doesn't have to be part of the error message or code. I wouldn't want to make design decisions based on the hypothesis that this is confusing to users without clear evidence that it is (e.g. a history of users filing issues on mypy about that error code or message); personally I find it intuitive.

I don't think we should return an Option or Result from member(), for the reason @AlexWaygood mentions above: we can have a possibly-unbound name (currently represented as a union with Unbound), and it's not clear how that should look in a binary representation.

We could design our own representation for "Type plus definitely-bound/maybe-unbound/definitely-unbound state" and use that instead of Type::Unbound. This will complicate some APIs and I think be somewhat less efficient, but it would have the advantage that it would force handling Unbound early, which I think is desirable. In general I don't think we want Unbound escaping from the scope where it originated; it should instead result in a diagnostic and be eliminated from the type (replaced with Unknown if we have no other type information; probably just disappear from the type if we do.) I do think this alternative to Type::Unbound is worth exploring and seeing what kind of impact it will have.

I was thinking that we would push diagnostics inside member() to simplify the handling of Unbound, and I think I still favor that approach. If we do want to make distinctions in error codes/messages based on context from the caller of member(), I think we can always pass in context to make that possible.

Here I think describing it as a failed attribute access would actually be much clearer than the vague "Could not resolve" which doesn't really communicate anything! This message also doesn't currently clarify that 'a' is a module (though it could be taken as implied.) I actually can't think of an error message here that I think would be better than "Module a has no attribute thing."

Do people really think of from collections import deque as an "attribute access" of deque from the collections module? I certainly don't. Of course that's what's happening under the hood, but even at runtime this detail is hidden -- the import fails with ImportError rather than AttributeError, and there's no mention of attempting to access an attribute:

% python -c "from collections import foo"                
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: cannot import name 'foo' from 'collections' (/Users/alexw/.pyenv/versions/3.12.4/lib/python3.12/collections/__init__.py)

I think when we issue the error on the import in a.py, we should set the type of foo in a to Unknown, not Unbound, which should avoid this?

Why not just emit the diagnostics inside this function, return an Option, and avoid the need for ModuleResolutionError?

I don't think we should be emitting a diagnostic at all if the imported symbol is of type Unknown. Unknown might result from an error in the other module, but in that case the diagnostic should occur in the other module; importing a value of unknown type is not an error.

I think just removing this clause would fix the ignored test; what would it break?

Yeah, the only reason we have to check for Unknown here is that we intentionally replaced Unbound with Unknown just a few lines above. If we wait to do it after, the bug in the ignored test goes away, and all tests pass:

diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs
index 173c957d1..c4ff40220 100644
--- a/crates/red_knot_python_semantic/src/types.rs
+++ b/crates/red_knot_python_semantic/src/types.rs
@@ -451,9 +451,6 @@ mod tests {
         );
     }

-    #[ignore = "\
-A spurious second 'Unresolved import' diagnostic message is emitted on `b.py`, \
-despite the symbol existing in the symbol table for `a.py`"]
     #[test]
     fn resolved_import_of_symbol_from_unresolved_import() {
         let mut db = setup_db();
diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs
index 9b183727e..28dc8f2e3 100644
--- a/crates/red_knot_python_semantic/src/types/infer.rs
+++ b/crates/red_knot_python_semantic/src/types/infer.rs
@@ -1012,15 +1012,9 @@ impl<'db> TypeInferenceBuilder<'db> {
             asname: _,
         } = alias;

-        // If a symbol is unbound in the module the symbol was originally defined in,
-        // when we're trying to import the symbol from that module into "our" module,
-        // the runtime error will occur immediately (rather than when the symbol is *used*,
-        // as would be the case for a symbol with type `Unbound`), so it's appropriate to
-        // think of the type of the imported symbol as `Unknown` rather than `Unbound`
         let member_ty = module_ty
             .unwrap_or(Type::Unbound)
-            .member(self.db, &Name::new(&name.id))
-            .replace_unbound_with(self.db, Type::Unknown);
+            .member(self.db, &Name::new(&name.id));

         if matches!(module_ty, Err(ModuleResolutionError::UnresolvedModule)) {
             self.add_diagnostic(
@@ -1032,7 +1026,7 @@ impl<'db> TypeInferenceBuilder<'db> {
                     module.unwrap_or_default()
                 ),
             );
-        } else if module_ty.is_ok() && member_ty.is_unknown() {
+        } else if module_ty.is_ok() && member_ty.is_unbound() {
             self.add_diagnostic(
                 AnyNodeRef::Alias(alias),
                 "unresolved-import",
@@ -1044,6 +1038,8 @@ impl<'db> TypeInferenceBuilder<'db> {
             );
         }

+        let member_ty = member_ty.replace_unbound_with(self.db, Type::Unknown);
+
         self.types.definitions.insert(definition, member_ty);
     }

Thanks, I think you're right that this fixes the tests I added (and skipped) in this PR. Here's an interesting edge case that you might not have considered, though. What should we do for something like this?

# foo.py
for x in range(0):
    pass

# bar.py
from foo import x

Running python bar.py fails with ImportError at runtime because x is not defined in foo.py (due to the iterable being empty), but neither mypy nor pyright catches this. Arguably we should infer the type of x after the for loop has completed as being int | Unbound -- should we flag any union that includes Unbound as one of the elements with an unresolved-import diagnostic? Probably not, I don't think -- the import might succeed, it just also might not. So let's say (for argument's sake) we use the error code import-possibly-unbound for that.

So then what if we have something like this, where the symbol definitely exists in foo.py, but is also definitely unbound? (Mypy and pyright do both catch this one.) Should that have the error code unresolved-import, or import-possibly-unbound?

# foo.py
x: int

# bar.py
from foo import x

These are somewhat obscure edge cases, so I think what you propose here is a strict improvement on what I implemented in this PR; I'll make a fixup PR to address your suggestions. But my point is that we may still at some point need some context propagated regarding the cause of the error -- whether that error is represented as Unknown or Unbound.

Yeah, great points and great questions! I had the x: int case in mind last night when thinking again about representation of unbound-ness in a different comment on this PR.

I agree that maybe-unbound is a different error class than definitely-unbound (and arguable whether maybe-unbound should trigger an error on import at all.)

I think we do want a representation of "definitely unbound and typed" that we use for imports, so e.g. we can error if you import from x: int (as long as it's not a stub), but we can still type check the importing module treating x as int rather than Unknown. (Not sure how important this is, but it seems marginally better.) I think this can be handled by declared vs inferred types.

What's less clear to me is if we need a representation of "definitely unbound and typed" for inferred types. It would mean that within a scope we could also check code following x: int and emit diagnostics both for "x is not bound" and for use of x that isn't valid for an int. Maybe that's useful?

Like I mentioned above in a different comment, I'm pretty open to exploring other representations of unbound to make it orthogonal to types; the main question for me is if we can avoid it regressing in perf, and if not, do we gain enough from it to be worth the regression?

I don't see the advantage of returning a Result vs just emitting the diagnostic directly in this method, as the previous code did.